Expand assert-pure.ql to cover common and be a path-problem query#2455
Expand assert-pure.ql to cover common and be a path-problem query#2455robertbrignull merged 3 commits intomainfrom
Conversation
aeisenberg
left a comment
There was a problem hiding this comment.
This is really neat. Thanks for implementing.
adityasharad
left a comment
There was a problem hiding this comment.
Neat! A couple of passing suggestions.
| class VSCodeImport extends AstNode { | ||
| VSCodeImport() { this.(Import).getImportedPath().getValue() = "vscode" } | ||
| } |
There was a problem hiding this comment.
Since you know the type you need, might as well declare it rather than casting.
| class VSCodeImport extends AstNode { | |
| VSCodeImport() { this.(Import).getImportedPath().getValue() = "vscode" } | |
| } | |
| class VSCodeImport extends Import { | |
| VSCodeImport() { this.getImportedPath().getValue() = "vscode" } | |
| } |
There was a problem hiding this comment.
Changing it to Import gets some errors because Import is an abstract class.
I assume this was the reason it was like this originally. However, I should be able to change it to ImportDeclaration because all of the imports in our codebase are import rather than require. My understanding is that means we can use ImportDeclaration and not miss anything. At least right now it doesn't change the results.
There was a problem hiding this comment.
Oof I didn't realise Import was abstract.
Changing it to extends ImportDeclaration is a good specific solution.
If you want something more generic, try the (relatively new) instanceof keyword: class VSCodeImport extends AstNode instanceof Import. instanceof says we are creating a subtype of Import that inherits all its methods, but does not add to its set of values as an abstract class.
| m.getAnImportedModule*().getAnImport() = v | ||
| select m, "This module is not pure: it has a transitive dependency on the vscode API imported $@", v, "here" | ||
| m.getFile() instanceof PureFile and | ||
| getANonTypeOnlyImport(getANonTypeOnlyImportedModule*(m)) = v |
There was a problem hiding this comment.
Minor: this looks correct, but it is less confusing if you can easily see the relationship between edges and the select clause. Right now I think each getANonTypeOnlyImportedModule step actually contains two edges(importing module -> import -> imported module) and the getANonTypeOnlyImport step is one edge (importing module -> import).
I think you could even write this as edges+(m, v) and it would work because of the typing of v, but verify that to be sure!
There was a problem hiding this comment.
Ooh, good point. Thanks for saying this. I agree it's less confusing if it's clear the relation to edges, and we can just do edges+(m, v) because of the typing of v.
Expands the
assert-pure.qlquery in a couple of ways:src/commondirectory as well assrc/pure. Stuff in this directory should also be independent of vscode, unless it is in thesrc/common/vscodedirectory.path-problemquery. My aim here is to make this query easier to use, by explaining where an import comes from. It can otherwise be a bit hard to understand when the import chain gets more than a couple of files.The query now has 10 alerts 🎉 😱
All of these alerts are because of logging, either from importing
extLoggerorOutputChannelLoggerorshowAndLogErrorMessage.The reason there's 6 alerts for
selection-commands.tsis because there's 6 separate vscode imports that it transitively imports, via the gianthelpers.tsfile. I believe there's not a good way to compress these into a single alert without losing the information of which import it's connected to.Checklist
ready-for-doc-reviewlabel there.